fix: downmerge from main to dev - #325
Merged
Merged
Conversation
…ate networking configuration
…ng application images
- ps1: move WAF ACR relaxation inside try{} so finally{} always restores
the locked-down state after any partial change
- ps1: check each restore command's exit code and warn per-step
- containerRegistry.bicep: update publicNetworkAccess description to reflect
WAF-disabled-at-rest behavior with script-driven temporary enablement
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iner deployment scripts
chore: Update Owners ID for Vinay and Prajwal
…h scripts for container images
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…r registry module
VishalSh-Microsoft
requested review from
Avijit-Microsoft,
Roopan-Microsoft,
aniaroramsft,
dgp10801,
nchandhi,
sethsteenken and
toherman-msft
as code owners
July 17, 2026 09:45
Contributor
There was a problem hiding this comment.
Pull request overview
This PR downmerges changes that shift image build/push out of the default azd up flow and into explicit post-deployment scripts, while updating infrastructure defaults to provision a dedicated ACR per deployment and improving private-networking (WAF) behavior for ACR.
Changes:
- Stop automatically building/pushing container images during
azddeployment; guide users to runacr_build_push.*manually. - Update infra defaults (image tag, placeholder image, dedicated ACR + ACR private endpoint/DNS support in WAF mode).
- Refresh deployment documentation and CODEOWNERS accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/acr_build_push.sh | Adds subscription pinning + WAF ACR “open/close” behavior; updates logging/tag defaults. |
| scripts/acr_build_push.ps1 | Mirrors bash script improvements for Windows, including WAF-safe ACR toggling. |
| infra/modules/containerRegistry.bicep | Adds conditional ACR private endpoint + DNS zone group and WAF-aligned registry policies. |
| infra/main.bicep | Updates image defaults and wires private endpoint/DNS inputs into the container registry module. |
| infra/main.json | Generated ARM reflecting dedicated ACR provisioning, placeholder images, and private DNS zone additions. |
| docs/DeploymentGuide.md | Adds a new step to manually build/push images after deployment and updates step references. |
| azure.yaml | Removes automatic image build hook and replaces it with user-facing instructions. |
| .github/CODEOWNERS | Updates default owner handles. |
Comments suppressed due to low confidence (4)
scripts/acr_build_push.sh:99
- If AZURE_SUBSCRIPTION_ID cannot be resolved (e.g., user runs the script without azd and without that env var), the script silently falls back to whatever Azure CLI subscription is currently active. Since the script explicitly calls out the risk of targeting the wrong subscription, it should warn when it can’t pin the subscription.
scripts/acr_build_push.sh:69 - The early "missing required deployment values" validation only checks ACR name and resource group. If the container app names are missing (and azd isn’t available to backfill them), the script will still do the remote builds and only fail later during update_app, wasting time/cost. Validate the required app-name outputs up-front so the script fails fast before building images.
scripts/acr_build_push.ps1:99 - If AZURE_SUBSCRIPTION_ID can’t be resolved (e.g., script run outside azd), the script silently proceeds using the Azure CLI’s current subscription. Given the risk called out in the comment above, emit a warning when the subscription can’t be pinned so users don’t accidentally build/update resources in the wrong subscription.
scripts/acr_build_push.ps1:169 - If container app names are missing, the script will still run the (potentially slow/costly) remote builds and only fail later inside Update-App. Fail fast before building images by validating the required container app name outputs right before the build step.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
scripts/acr_build_push.sh:66
- The script treats
AZURE_SUBSCRIPTION_IDas “required” for the azd fallback (and later relies on it to pin the Azure CLI subscription / correctly detect WAF), but the final required-values validation only checks ACR name + resource group. IfAZURE_SUBSCRIPTION_IDis missing (e.g., running outside azd hooks), the script will proceed using whatever Azure CLI subscription is currently active, which defeats the intent and can target the wrong subscription or fail to detect WAF mode.
scripts/acr_build_push.ps1:97 AZURE_SUBSCRIPTION_IDis treated as optional here (the subscription is only set when it’s non-empty), but the script’s behavior depends on being scoped to the correct subscription (ACR updates, RG tag lookup for WAF detection, builds/updates). If the env var is missing, the script will run against whatever Azure CLI subscription is currently active, which can build/update the wrong deployment.
…ent-changes fix: remove local deployment changes
Roopan-Microsoft
approved these changes
Jul 20, 2026
Avijit-Microsoft
approved these changes
Jul 20, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
scripts/acr_build_push.sh:64
AZURE_SUBSCRIPTION_IDis treated as required (and used to pin the Azure CLI subscription), but it is never included in the missing-value validation. Ifazdis unavailable (or doesn’t provide it), the script can proceed without pinning the subscription, which can causeaz acr build/az containerapp updateto run against the wrong subscription or fail later with less-clear errors. ValidateAZURE_SUBSCRIPTION_IDalongside the other required deployment values (or make it consistently optional).
scripts/acr_build_push.ps1:38- This block treats
$SubscriptionIdas one of the “required values” that triggersazd env get-values, but the script never errors if it remains unset and only pins the subscription when it’s non-empty. Either validate$SubscriptionIdas truly required, or remove it from this condition so it’s consistently optional.
Comment on lines
+877
to
+878
| "privateEndpointSubnetResourceId": "[if(parameters('enablePrivateNetworking'), createObject('value', resourceId(resourceGroup().name, 'Microsoft.Network/virtualNetworks/subnets', format('vnet-{0}', variables('solutionSuffix')), 'backend')), createObject('value', ''))]", | ||
| "privateDnsZoneResourceId": "[if(parameters('enablePrivateNetworking'), createObject('value', resourceId(resourceGroup().name, 'Microsoft.Network/privateDnsZones', 'privatelink.azurecr.io')), createObject('value', ''))]", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request updates the deployment process and infrastructure configuration for container images, with a focus on improving clarity, security, and automation. The main changes are: container images are no longer built and pushed automatically during deployment; instead, clear instructions and scripts are provided for manual execution. Infrastructure defaults and documentation are updated to reflect these changes, and private networking for the Azure Container Registry (ACR) is enhanced.
Deployment process and documentation:
azure.yaml) so container images are not built/pushed automatically; users are now instructed to runacr_build_push.shoracr_build_push.ps1manually, and deployment logs reflect this change. [1] [2]docs/DeploymentGuide.md) to add a new step for manually building and pushing images after deployment, with clear instructions for both PowerShell and Bash. Step references in later instructions are updated accordingly. [1] [2]Infrastructure configuration and security:
"latest_v2"to"latest"and updated the placeholder container image to use the Azure Container Apps hello-world image, reflecting the new script-based deployment flow. [1] [2]privatelink.azurecr.ioto the list of managed private DNS zones and updated DNS zone index variables accordingly, improving private endpoint support for ACR. [1] [2] [3] [4]Other improvements:
containerRegistryEndpointparameter to be empty by default and marked as deprecated, clarifying that each deployment now provisions its own dedicated ACR..github/CODEOWNERS.Does this introduce a breaking change?
Golden Path Validation
Deployment Validation